Fix key prop type #74

mununki · 2022-10-21T17:42:30Z

This PR fixed #73

cristianoc · 2022-10-23T05:52:21Z

src/React.res

-external jsx: (component<'props>, 'props) => element = "jsx"
+external jsxKeyed: (component<'props>, 'props, string) => element = "jsx"
+
+let jsx = (~key=?, component, props) =>


Wondering whether this can be achieved directly with an external.
Btw where is the documentation that one can pass the key as third argument to the react jsxfunction?

external jsxKeyed: (component<'props>, 'props, ~key: option<string>=?) => element = "jsx"

Like this? Shouldn't we have to move the labelled key argument position to first?

Yes that can't be done because it needs to be before mandatory arguments.
Unless one adds a fake additional mandatory argument of type unit perhaps.

Would this work?

type was wrong_edited_to_fix_type_of_key

external jsx: (component<'props>, 'props, ~key: string=?, @ignore unit) => element = "jsx"

Would this work?

external jsx: (component<'props>, 'props, ~key: option<string>=?, @ignore unit) => element = "jsx"

I don't think it is working.

I think for cleanest generated output, one needs to expose 2 functions:
jsx without a key argument, and jsxKeyed with a mandatory 3rd argument of type option<string>.
Then at call site the PPX would call jsx when key is not passed, and would call jsxKeyed when key is passed.

Btw, I couldn't find the doc about the third argument as key, but I found it from the source code. https://github.com/facebook/react/blob/8e2bde6f2751aa6335f3cef488c05c3ea08e074a/packages/react/src/jsx/ReactJSXElement.js#L210

I think the key argument needs to be optional not option<string> as mandatory. If it is option<string> as mandatory, then users should use it.

<C key=Some("k") />

I think the key argument needs to be optional not option<string> as mandatory. If it is option<string> as mandatory, then users should use it.

<C key=Some("k") />

It does not have to be optional. The PPX can take care of passing the value in the appropriate way.
But it might be simpler to make it optional so the call PPX only needs to do one special thing: pass a final ().

I think for cleanest generated output, one needs to expose 2 functions: jsx without a key argument, and jsxKeyed with a mandatory 3rd argument of type option<string>. Then at call site the PPX would call jsx when key is not passed, and would call jsxKeyed when key is passed.

This would be easy fix, I'll fix the binding and make a PR for PPX.

mununki · 2022-10-23T07:34:59Z

rescript-lang/syntax#693 (comment)

Maybe we need to revert the addKeyProp function back to avoid the manipulating props object which user made.

cristianoc · 2022-10-23T07:39:09Z

Exploring the opportunity to use cloneElement. Assuming I understood how this works https://reactjs.org/docs/react-api.html#cloneelement

Instead of this:

React.createElementWithKey(~key="k", V4CA.make, {})

one would generate this:

React.cloneElement(React.createElement(V4CA.make, {}), {"key": "k"})

So one would rely on 2 pure bindings without any runtime in rescript-react.

mununki · 2022-10-23T07:52:23Z

Exploring the opportunity to use cloneElement. Assuming I understood how this works https://reactjs.org/docs/react-api.html#cloneelement

Instead of this:
React.createElementWithKey(~key="k", V4CA.make, {})
one would generate this:
React.cloneElement(React.createElement(V4CA.make, {}), {"key": "k"})
So one would rely on 2 pure bindings without any runtime in rescript-react.

Good idea!
But we need to consider the key as optional argument for this expression <C ?key />

cknitt · 2022-10-23T08:00:18Z

So one would rely on 2 pure bindings without any runtime in rescript-react.

That's certainly an advantage, but it will be even more inefficient than the Object.assign solution, so we are moving away further and further from the "zero cost" idea.

Here is the implementation of cloneElement: https://github.com/facebook/react/blob/8e2bde6f2751aa6335f3cef488c05c3ea08e074a/packages/react/src/ReactElement.js#L486

cknitt · 2022-10-23T08:13:17Z

I think we would only need to do make sure that the props object is copied in the

<A {...props} key />

case. (In JS, {...props} would copy the props object, too.)

Then we can use the

Obj.magic(props)["key"] = key

solution for all cases.

cristianoc · 2022-10-23T08:41:11Z

React.createElement seems likely to become legacy over time. I would not worry too much about fine tuning corner cases, especially imperative update vs obj assign in the rare case where key is used. Perhaps just reverting the change, and always use obj assign is simplest.

For the jsx mode, that seems the way things will trend over time. And it makes sense to optimise the code generated in that case. In particular, it would be nice to introduce zero dependencies, so one can look at the generated code and understand it in isolation.

cknitt · 2022-10-23T09:11:37Z

The case where key is used is actually not that rare, it basically needs to be used for every list you render (React.array), and there it will be used for each element.

But yes, if we view this as "legacy JSX transform support", then that's probably ok, as we do not have the same issue with key with the new JSX transform.

mununki · 2022-10-23T09:50:58Z

case. (In JS, {...props} would copy the props object, too.)

In this case, {...p} is not copy the object. The PPX uses just p as props in case without other props. So, this case p is not copied and manipulated.

mununki · 2022-10-23T09:52:36Z

React.createElement seems likely to become legacy over time. I would not worry too much about fine tuning corner cases, especially imperative update vs obj assign in the rare case where key is used. Perhaps just reverting the change, and always use obj assign is simplest.

For the jsx mode, that seems the way things will trend over time. And it makes sense to optimise the code generated in that case. In particular, it would be nice to introduce zero dependencies, so one can look at the generated code and understand it in isolation.

The case where key is used is actually not that rare, it basically needs to be used for every list you render (React.array), and there it will be used for each element.

But yes, if we view this as "legacy JSX transform support", then that's probably ok, as we do not have the same issue with key with the new JSX transform.

I agree you both. I'll revert the addKeyProp.

This reverts commit 2f48356.

mununki · 2022-10-23T11:25:18Z

I think for cleanest generated output, one needs to expose 2 functions: jsx without a key argument, and jsxKeyed with a mandatory 3rd argument of type option<string>. Then at call site the PPX would call jsx when key is not passed, and would call jsxKeyed when key is passed.

This would be easy fix, I'll fix the binding and make a PR for PPX.

I've fixed it rescript-lang/syntax#694 234798b

cristianoc · 2022-10-23T12:43:51Z

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog

+## 0.11.0-rc.3
+
+- Renamed `React.jsx(s)`, `ReactDOM.jsx(s)` to `React.jsx(s)NotKeyed`, `ReactDOM.jsx(s)NotKeyed`.


Needs update.

Sorry for missing changelog and tests too often 😅
Updated b8fa0f3

fix-key-type

5e09771

mununki mentioned this pull request Oct 21, 2022

Version 0.11.0-rc.2 #72

Merged

mununki added 2 commits October 22, 2022 11:42

rename jsx binding

10f6400

fix key type for ReactDOM jsx runtime

9b0fe1b

cknitt mentioned this pull request Oct 22, 2022

Fix react comp key type rescript-lang/syntax#693

Merged

mununki added 2 commits October 23, 2022 00:53

update changelog

adc1a55

add key imperatively

2f48356

cristianoc reviewed Oct 23, 2022

View reviewed changes

cristianoc previously approved these changes Oct 23, 2022

View reviewed changes

Revert "add key imperatively"

3f0f2e9

This reverts commit 2f48356.

mununki dismissed cristianoc’s stale review via 3f0f2e9 October 23, 2022 09:54

change jsx runtime binding

234798b

cristianoc reviewed Oct 23, 2022

View reviewed changes

update changelog

b8fa0f3

cristianoc approved these changes Oct 23, 2022

View reviewed changes

cristianoc merged commit fb232ec into master Oct 23, 2022

cristianoc deleted the fix-key-type branch October 23, 2022 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix key prop type #74

Fix key prop type #74

mununki commented Oct 21, 2022

cristianoc Oct 23, 2022

mununki Oct 23, 2022 •

edited

Loading

cristianoc Oct 23, 2022

cristianoc Oct 23, 2022 •

edited

Loading

mununki Oct 23, 2022

cristianoc Oct 23, 2022

mununki Oct 23, 2022 •

edited

Loading

mununki Oct 23, 2022

cristianoc Oct 23, 2022

mununki Oct 23, 2022

mununki commented Oct 23, 2022 •

edited

Loading

cristianoc commented Oct 23, 2022

mununki commented Oct 23, 2022

cknitt commented Oct 23, 2022

cknitt commented Oct 23, 2022

cristianoc commented Oct 23, 2022

cknitt commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

cristianoc Oct 23, 2022

mununki Oct 23, 2022

Fix key prop type #74

Fix key prop type #74

Conversation

mununki commented Oct 21, 2022

Choose a reason for hiding this comment

mununki Oct 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc Oct 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mununki Oct 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mununki commented Oct 23, 2022 • edited Loading

cristianoc commented Oct 23, 2022

mununki commented Oct 23, 2022

cknitt commented Oct 23, 2022

cknitt commented Oct 23, 2022

cristianoc commented Oct 23, 2022

cknitt commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mununki Oct 23, 2022 •

edited

Loading

cristianoc Oct 23, 2022 •

edited

Loading

mununki Oct 23, 2022 •

edited

Loading

mununki commented Oct 23, 2022 •

edited

Loading